-
Notifications
You must be signed in to change notification settings - Fork 639
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support $beforeUpdate() mutations in upsertGraph() #2452
Conversation
promise = chainOperationHooks(promise, builder, 'onBefore2'); | ||
|
||
promise = chainHooks(promise, builder, builder.context().runBefore); | ||
promise = chainHooks(promise, builder, builder.internalContext().runBefore); | ||
|
||
promise = chainOperationHooks(promise, builder, 'onBefore2'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am a bit afraid of this change - could you tell more about why the execution order of hooks had to be changed to fix the issue, please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need runBefore
to run after onBefore2()
in UpdateOperation
which triggers the execution of $beforeUpdate()
. I tried it and it didn't break any tests. I looked at afterExecute()
, and it has the same sequence.
In the end, this is just a proof of concept more than anything for now. I do think that the behavior it produces is more according to what I would expect though.
function afterExecute(builder, result) {
let promise = Promise.resolve(result);
promise = chainOperationHooks(promise, builder, 'onAfter1');
promise = chainOperationHooks(promise, builder, 'onAfter2');
promise = chainHooks(promise, builder, builder.context().runAfter);
promise = chainHooks(promise, builder, builder.internalContext().runAfter);
promise = chainOperationHooks(promise, builder, 'onAfter3');
return promise;
}
027868d
to
0b9499a
Compare
Hey @lehni 👋 Are you planning to merge and release this soon? We are looking forward to update finally to the latest objection version 😬 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've just run our test suites on a project which uses upsertGraph
a lot, with the changes from this PR applied and nothing broke. I also didn't observe any performance degradation, so it looks good 👍
Good news! I still need to write a test for this, but will merge it soon. |
0b9499a
to
4df4955
Compare
I have improved the code to detect the presence of |
4df4955
to
bf98cfb
Compare
bf98cfb
to
3940e7d
Compare
Closes #2233